8368781: PerfMemory - make issues more transparent #27602
Conversation
|
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
Webrevs
|
|
I think adding a new diagnostic flag named |
Might be an option ; PrintMiscellaneous is used besides perfMemory also at some more places , e.g. C2 JIT. Would it be okay to offer this in product JVMs ? (Verbose is even harder to say, because it is used more) |
|
PerfMemory should be converted to Unified Logging (UL). We don't want any new flags for ad-hoc logging. |
Guess this makes sense, do you have some special UL category in mind? Maybe 'os' ? Or a new one ? |
|
We already have some UL in PerfMemory:
but for some reason that effort completely ignored the PrintMiscellaneous/Verbose "warnings". To keep things simple I suggest the following pattern: Of course you proposed changes as a potential consumer of this information, so how would you like to procure it? |
I think my support colleague would set the needed UL flags in case her wants to find out more about perfmem issues. |
|
@dholmes-ora while looking into the Windows perfmem code, I noticed we are using SetSecurityDescriptorControl jdk/src/hotspot/os/windows/perfMemory_windows.cpp Line 1016 in 1653999 but we use still handling for ancient Windows versions (we use dynamic handling of the function at runtime), should we remove this ? |
|
Do we need to include more? It seems not to work in e.g. the zero of minimal builds ? |
Yes please file a separate JBS issue for that |
I created |
| if (log_is_enabled(Debug, perf)) { | ||
| log_debug(perf)("Could not commit PerfData memory"); |
There was a problem hiding this comment.
You don't need the log_is_enabled check with a single logging statement as the log_debug is a macro that already contains the check.
There was a problem hiding this comment.
That's good, saves us quite a few lines !
| warning("Invalid performance data file path name specified, "\ | ||
| "fall back to a default name"); | ||
| } | ||
| log_debug(perf)("Invalid performance data file path name specified, "\ |
There was a problem hiding this comment.
Btw is the '\'here really needed ? We had it before but most strings over multiple lines do not use it, from what I see ?
There was a problem hiding this comment.
No it isn't needed - string literals across new-lines are concatenated. You don't need an explicit line continuation character. Not that it makes any real difference.
There was a problem hiding this comment.
I've suggested reformatting most of these anyway as they don't need to be so short in many cases.
| log.print_cr("Could not determine user name: %s", | ||
| p->pw_name == nullptr ? "pw_name = null" : "pw_name zero length"); |
There was a problem hiding this comment.
| log.print_cr("Could not determine user name: %s", | |
| p->pw_name == nullptr ? "pw_name = null" : "pw_name zero length"); | |
| log.print_cr("Could not determine user name: %s", | |
| p->pw_name == nullptr ? "pw_name = null" : "pw_name zero length"); |
Fix indent
| log_debug(perf)("Could not unlink shared memory backing" | ||
| " store file %s : %s", path, os::strerror(errno)); |
There was a problem hiding this comment.
| log_debug(perf)("Could not unlink shared memory backing" | |
| " store file %s : %s", path, os::strerror(errno)); | |
| log_debug(perf)("Could not unlink shared memory backing store file %s : %s", | |
| path, os::strerror(errno)); |
| log_debug(perf)("could not get attributes for file %s: " | ||
| " lasterror = %d", path, lasterror); |
There was a problem hiding this comment.
| log_debug(perf)("could not get attributes for file %s: " | |
| " lasterror = %d", path, lasterror); | |
| log_debug(perf)("could not get attributes for file %s: lasterror = %d", | |
| path, lasterror); |
| log_debug(perf)("Could not unlink shared memory backing" | ||
| " store file %s : %s", path, os::strerror(errno)); |
There was a problem hiding this comment.
| log_debug(perf)("Could not unlink shared memory backing" | |
| " store file %s : %s", path, os::strerror(errno)); | |
| log_debug(perf)("Could not unlink shared memory backing store file %s : %s", | |
| path, os::strerror(errno)); |
| } | ||
| DWORD lastError = GetLastError(); | ||
| if (lastError != ERROR_INVALID_PARAMETER) { | ||
| log_debug(perf)("OpenProcess failed: %d", GetLastError()); |
There was a problem hiding this comment.
| log_debug(perf)("OpenProcess failed: %d", GetLastError()); | |
| log_debug(perf)("OpenProcess failed: %d", lastError); |
| log_debug(perf)("SetSecurityDescriptorControl failure:" | ||
| " lasterror = %d", GetLastError()); |
There was a problem hiding this comment.
| log_debug(perf)("SetSecurityDescriptorControl failure:" | |
| " lasterror = %d", GetLastError()); | |
| log_debug(perf)("SetSecurityDescriptorControl failure: lasterror = %d", GetLastError()); |
| log_debug(perf)("InitializeSecurityDescriptor failure: " | ||
| "lasterror = %d", GetLastError()); |
There was a problem hiding this comment.
| log_debug(perf)("InitializeSecurityDescriptor failure: " | |
| "lasterror = %d", GetLastError()); | |
| log_debug(perf)("InitializeSecurityDescriptor failure: lasterror = %d", GetLastError()); |
| log_debug(perf)("AllocateAndInitializeSid failure: " | ||
| "lasterror = %d", GetLastError()); |
There was a problem hiding this comment.
| log_debug(perf)("AllocateAndInitializeSid failure: " | |
| "lasterror = %d", GetLastError()); | |
| log_debug(perf)("AllocateAndInitializeSid failure: lasterror = %d", GetLastError()); |
| log_debug(perf)("AllocateAndInitializeSid failure: " | ||
| "lasterror = %d", GetLastError()); |
There was a problem hiding this comment.
| log_debug(perf)("AllocateAndInitializeSid failure: " | |
| "lasterror = %d", GetLastError()); | |
| log_debug(perf)("AllocateAndInitializeSid failure: lasterror = %d", GetLastError()); |
| warning("Invalid performance data file path name specified, "\ | ||
| "fall back to a default name"); | ||
| } | ||
| log_debug(perf)("Invalid performance data file path name specified, "\ |
There was a problem hiding this comment.
I've suggested reformatting most of these anyway as they don't need to be so short in many cases.
3 participants
Currently issues with perfMemory like problems with the secure tmp subdirectory creation are not very transparent in release JVMs.
There exists some warnings traces but they are behind develop flags like Verbose so only available in debug JVMs.
We could (in case of issues) store some information and write it later into hsinfo/hserr files ; or make the existing warnings available too in release JVMs.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27602/head:pull/27602$ git checkout pull/27602Update a local copy of the PR:
$ git checkout pull/27602$ git pull https://git.openjdk.org/jdk.git pull/27602/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27602View PR using the GUI difftool:
$ git pr show -t 27602Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27602.diff
Using Webrev
Link to Webrev Comment